-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(link-editor): update behaviour so that we can save on link collection items #863
Conversation
1. refactor link modal so it uses context 2. update other components to use the new api
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Datadog ReportBranch report: ✅ 0 Failed, 184 Passed, 34 Skipped, 39.63s Total Time |
will this be confusing UX? because it's not obvious to the end user why and when it resets |
yeap, getting @sehyunidaaa to double check this but i thought this was intuitive because:
|
Just to confirm - we had to use this logic of resetting only when there's a change because it's difficult to maintain the input across different tabs right (E.g., if file has an attachment and link also has a link, we can't keep the two) But I agree with Chin on this, I think once the user inputs something in a different tab then it's a signal that they want to link something else |
...atures/editing-experience/components/form-builder/renderers/controls/JsonFormsRefControl.tsx
Outdated
Show resolved
Hide resolved
apps/studio/src/features/editing-experience/components/LinkEditor/LinkHrefEditor.tsx
Outdated
Show resolved
Hide resolved
apps/studio/src/features/editing-experience/components/LinkEditor/LinkHrefEditor.tsx
Outdated
Show resolved
Hide resolved
apps/studio/src/features/editing-experience/components/LinkEditor/LinkHrefEditor.tsx
Outdated
Show resolved
Hide resolved
linkTypes: Record< | ||
string, | ||
{ | ||
icon: IconType | ||
label: Capitalize<LinkTypes> | ||
} | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use LinkTypeMapping
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i actually tried this and it didn't quite work. this is because when we say Record<LinkTypes, string>
, actually what we mean is a 1:1 mapping (ie, every type in LinkType
must have a corresponding string) which isn't quite what we want here. what we want is "some subset of LinkTypes to string" - i considered using Partial<Record<LinkTypes, string>>
, but unfortunately that means that every type is now optional. hence, i just went iwth string for now
Problem
This PR fixes an issue with ISOM-1654 (and more deeply, the link editor)
Closes ISOM-1654
Solution
LinkEditorContext
that all the child components read from. we do this so that the state can be unified for what the href currently is + the currently active link itemonChange
to the context and have the children reference it. this isn't quite ideal yet because page/file elements live outside of the context but we're manually passing the sameonChange
+ callsite is limited, this shuold be okVideos + Screenshots
Screen.Recording.2024-11-07.at.1.00.27.PM.mov
overflow now